Skip to content

Conversation

@mcollina
Copy link
Collaborator

I've added support for async/await during get.concat().

This is a wip, and more intended to discuss rather than a proper direction.

The main concern is on the return type, which is in the form of { res, data }, so it uses a full object instead of positional arguments. This is also the main reason why util.promisify() cannot be used with get.concat().

It also make the module pass the > 100 lines :(.

(we can also ship require('simple-get/promise') as a second entry point).

@feross
Copy link
Owner

feross commented Apr 26, 2018

Thanks for the PR! I was planning to look into Promise support for the next major version. A couple thoughts:

  1. What do you think about dropping the callback interface entirely and releasing a new major version? I'm hesitant to support both interfaces and duplicate the full test suite across both patterns.

  2. Rather than returning an object { res, data }, let's just set the data on the response object as res.body. Then we can just return res.

Thoughts?

@mcollina
Copy link
Collaborator Author

What do you think about dropping the callback interface entirely and releasing a new major version?

I think supporting both is the way to go for any existing library. Switching from one to the other implies that all of your users will have to migrate from one to the other, or be stuck on a minor updates.

I'm hesitant to support both interfaces and duplicate the full test suite across both patterns.

I typically don't duplicate the tests. I go additive and just test the promise layer on top. I know it's more messy but it gets the job done and it does not break anybody.

I'm also +1 to add some async-await specific tests, just to validate how the user will see the API (see https://github.com/fastify/fastify/blob/master/test/async-await.test.js and https://github.com/fastify/fastify/blob/master/test/async-await.js).

Another option is to switch to res.body anyway (this is the main offender), and then recommend user to rely on https://nodejs.org/api/util.html#util_util_promisify_original.

@emilbayes
Copy link
Collaborator

+1 for this, as well as keeping both interfaces. Use simple-get as my main http request library, and using async/await in a callback context and vice versa is awkward at best

@roblav96
Copy link

roblav96 commented May 1, 2018

+1 for this merge! :D

@mcollina
Copy link
Collaborator Author

mcollina commented May 1, 2018

@feross would you like me to update this to use response.body instead? We might want to keep the wrapping object anyway, because we might want to return the req as well.

@feross
Copy link
Owner

feross commented May 10, 2018

@mcollina I want to think about this a bit more before deciding what to do.

@tahercool1
Copy link

Any updates on this?

@yaneony
Copy link

yaneony commented Mar 11, 2021

Still nothing since 2 years?!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants